[lts_03_2025] Use safe_add_size_t when sizing DPS transport response buffers (backport of #2738) - #2746
Conversation
* Use safe_add_size_t when sizing DPS transport response buffers The DPS HTTP, MQTT and AMQP transports sized their response buffers with malloc(len + 1). Compute the allocation size with safe_add_size_t (as already done elsewhere in the tree, e.g. uhttp) and fail the request if the addition saturates, instead of allocating a mis-sized buffer. Add a regression unit test per transport covering the saturating-length path. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Address review feedback - prov_transport_mqtt_common: drop duplicate http_proxy_io.h include - prov_transport_http_client: correct payload-allocation error message - amqp unit test: assert the error is surfaced to the register callback --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> (cherry picked from commit 70f9fa2)
The cherry-picked regression tests use IGNORED_ARG, which is only defined in newer umock-c. lts_03_2025 pins umock-c 504193e6 (Mar 2020), where that macro does not exist, so the three provisioning UT suites would fail to compile. Replace IGNORED_ARG with IGNORED_PTR_ARG, matching the macro already used throughout these test files on this branch. All nine occurrences are pointer arguments, so IGNORED_PTR_ARG is the correct substitution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
CI on this PR is blocked by broken infrastructure on
|
Local build and test verification — now doneI previously said this PR was unvalidated. I have since built a toolchain in my environment (gcc 12.2, cmake 3.25) and built and ran all three affected unit-test suites. Results:
196 tests, 0 failures. This confirms the The three new regression tests each ran and passed: The new tests are meaningful, not vacuousTo prove the tests actually exercise the vulnerable path, I reverted only the three Restoring the fix returns all three suites to green. So the tests genuinely detect the wrap-then-overflow, and the fix genuinely prevents it. Caveat on severity — unchangedThis demonstrates the pattern is dangerous once a length of CICI here will stay red until #2747 merges — this branch still carries the old pipeline that fails in |
There was a problem hiding this comment.
Pull request overview
Backport of mainline hardening to the lts_03_2025 branch to prevent size_t overflow when allocating DPS transport response buffers (HTTP/MQTT/AMQP), plus regression unit tests to ensure oversized lengths are rejected instead of causing heap corruption.
Changes:
- Replace
malloc(len + 1)sizing withsafe_add_size_t(len, 1)checks in DPS HTTP/MQTT/AMQP receive paths. - Add one regression UT per transport that injects a saturating
(size_t)-1length and asserts the error path is taken (no overflow/corruption). - Minor LTS test adaptation: use
IGNORED_PTR_ARGinstead ofIGNORED_ARGfor compatibility with the pinned umock-c.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| provisioning_client/src/prov_transport_http_client.c | Uses safe_add_size_t for reply buffer sizing and avoids dangling payload_data on free. |
| provisioning_client/src/prov_transport_mqtt_common.c | Uses safe_add_size_t for MQTT payload buffer sizing and rejects saturating lengths. |
| provisioning_client/src/prov_transport_amqp_common.c | Uses safe_add_size_t for AMQP message body buffer sizing and rejects saturating lengths. |
| provisioning_client/tests/prov_transport_http_client_ut/prov_transport_http_client_ut.c | Adds regression test for saturating content_len in HTTP receive callback. |
| provisioning_client/tests/prov_transport_mqtt_common_ut/prov_transport_mqtt_common_ut.c | Adds regression test for saturating MQTT payload length and validates error surfacing. |
| provisioning_client/tests/prov_transport_amqp_common_ut/prov_transport_amqp_common_ut.c | Adds regression test for saturating AMQP body length and validates error surfacing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (alloc_size == SIZE_MAX) | ||
| { | ||
| LogError("Failure sending http request"); | ||
| LogError("Failure invalid content length specified"); |
There was a problem hiding this comment.
Agreed on the substance; not changing it in this PR.
You are right that the value is not caller-specified. content_len is the length reported by the receive callback — in azure-uhttp-c it is BUFFER_length(recv_msg.msg_body), i.e. bytes actually received and buffered, not the Content-Length header. That distinction is exactly why this is hardening rather than a remotely reachable overflow, so the wording is worth correcting.
The reason not to do it here: this string is byte-identical to main, and "the three provisioning_client/src files are byte-identical to main at the allocation sites" is the main safety argument for this backport. Changing it would make the branches diverge and leave the same wording wrong on main.
Better fixed on main and allowed to flow down. Happy to open that PR.
| // A Content-Length of SIZE_MAX would wrap (content_len + 1) to 0; the transport must | ||
| // reject it instead of allocating a zero-length buffer and copying gigabytes past it. |
There was a problem hiding this comment.
Same conclusion as the log-message thread, and the comment is genuinely misleading.
The test drives g_on_http_reply_recv(...) directly with content_len = (size_t)-1, so it exercises the callback parameter, not header parsing. Referring to "a Content-Length of SIZE_MAX" implies a malicious header reaches this code path unchanged, which is not what happens — uhttp errors out when the received body length does not match, so content_len is bytes actually buffered.
Not changing it here only to keep this backport a clean cherry-pick of main; the comment is identical on main. Worth correcting there.
For the record, the test itself is sound — I verified it fails against the unfixed source: reverting the three src files to malloc(len + 1) makes all three suites abort with heap corruption, and restoring the fix returns 196/196 green.
Backports #2738 to
lts_03_2025, the only LTS branch still in maintenance (end date 2026-10-07 perreadme.md).Why
The DPS transports size their response buffers with
malloc(len + 1), so alenofSIZE_MAXwraps tomalloc(0)and the followingmemcpyruns off the allocation. #2738 fixed this onmainon 2026-07-23; the LTS branch still carries the unhardened pattern in all three files.Contents
70f9fa28cherry-picked unchanged. The threeprovisioning_client/srcfiles are byte-identical tomainat the allocation sites.IGNORED_ARG, which does not exist in the umock-c pinned by this branch (504193e6, March 2020). Without this the three provisioning UT suites fail to compile. All nine occurrences are pointer arguments, so they becomeIGNORED_PTR_ARG, matching the macro already used throughout these files here.Verification
Built and ran all three affected suites locally:
prov_transport_http_client_utprov_transport_mqtt_common_utprov_transport_amqp_common_utThe three new regression tests each ran and passed.
To confirm they are not vacuous, I reverted only the three
srcfiles to the unfixedmalloc(len + 1)and reran — all three suites crashed with heap corruption (munmap_chunk(): invalid pointer,malloc(): unaligned tcache chunk detected,malloc(): memory corruption (fast), core dumped). Restoring the fix returns them to green.Reachability
Hardening / defence in depth. I could not find a path by which a wire-provided length reaches a wrapping value:
content_lenisBUFFER_length(recv_msg.msg_body)in azure-uhttp-c, i.e. bytes actually received and buffered, not theContent-Lengthheader. uhttp errors on mismatch.payload->lengthcomes from the umqtt codec, whereprepareheaderDataInfo()rejectstotalLen > MAX_SEND_SIZEand Remaining Length fields longer than 4 bytes.binary_data.lengthoriginates fromamqp_binary.length, auint32_t; on 64-bituint32 + 1cannot wrapsize_t.The crash above is produced by test mocks injecting
SIZE_MAXdirectly.Notes
772a4f8b) already providessafe_add_size_t, macro identical tomain.5a48d3fd8).payload_data = NULLafterfree(), so the malloc-failure branch no longer leaves a dangling pointer.http_proxy_io.hinclude dropped fromprov_transport_mqtt_common.cwas genuinely duplicated here too (lines 17 and 19); one remains.